[SPARK-14123] [SPARK-14384] [SQL] Handle CreateFunction/DropFunction #12117

yhuai · 2016-04-01T21:56:31Z

What changes were proposed in this pull request?

This PR implements CreateFunction and DropFunction commands. Besides implementing these two commands, we also change how to manage functions. Here are the main changes.

FunctionRegistry will be a container to store all functions builders and it will not actively load any functions. Because of this change, we do not need to maintain a separate registry for HiveContext. So, HiveFunctionRegistry is deleted.
SessionCatalog takes care the job of loading a function if this function is not in the FunctionRegistry but its metadata is stored in the external catalog. For this case, SessionCatalog will (1) load the metadata from the external catalog, (2) load all needed resources (i.e. jars and files), (3) create a function builder based on the function definition, (4) register the function builder in the FunctionRegistry.
A UnresolvedGenerator is created. So, the parser will not need to call FunctionRegistry directly during parsing, which is not a good time to create a Hive UDTF. In the analysis phase, we will resolve UnresolvedGenerator.

This PR is based on @viirya's #12036

How was this patch tested?

Existing tests and new tests.

TODOs

[x] Self-review
[x] Cleanup
[x] More tests for create/drop functions (we need to more tests for permanent functions).
[ ] File JIRAs for all TODOs
[x] Standardize the error message when a function does not exist.

Conflicts: sql/core/src/main/scala/org/apache/spark/sql/execution/command/ddl.scala

Conflicts: sql/core/src/main/scala/org/apache/spark/sql/execution/SparkQl.scala

…nRegistry

…egistry loads permanent function.

…o try Hive's.

SparkQA · 2016-04-01T21:58:57Z

Test build #54723 has finished for PR 12117 at commit e0570cd.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2016-04-01T22:03:52Z

Test build #54724 has finished for PR 12117 at commit 0a1866c.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

andrewor14 · 2016-04-04T23:27:04Z

sql/core/src/main/scala/org/apache/spark/sql/execution/command/functions.scala

+ * alias: the class name that implements the created function.
+ * resources: Jars, files, or archives which need to be added to the environment when the function
+ *            is referenced for the first time by a session.
+ * isTemp: indicates if it is a temporary function.


can you follow the javadoc format of the other commands, something like:

* ... * The syntax of using this command in SQL is: * {{{ * SHOW FUNCTIONS [LIKE pattern] * }}}

The command format is pretty useful. Same in DropFunction

andrewor14 · 2016-04-05T00:34:44Z

Thanks @yhuai. This looks pretty good. I left a few suggestions on how the documentation, code readability and placement of functionality can be improved, but these are all relatively minor. Once you address the comments I will go ahead and merge this.

Also thanks @viirya for the original changes.

viirya · 2016-04-05T01:09:19Z

Thanks @yhuai for improving the original changes. This looks great now.

viirya · 2016-04-05T02:03:11Z

sql/hive/src/test/scala/org/apache/spark/sql/hive/HiveSparkSubmitSuite.scala

@@ -208,6 +261,118 @@ class HiveSparkSubmitSuite
  }
 }

+// This application is used to test defining a new Hive UDF (with an associated jar)


Yea. This is good!

viirya · 2016-04-05T02:11:47Z

sql/hive/src/main/scala/org/apache/spark/sql/hive/test/TestHive.scala

@@ -201,8 +201,13 @@ class TestHiveContext private[hive](
    }

    override lazy val functionRegistry = {
-      new TestHiveFunctionRegistry(
-        org.apache.spark.sql.catalyst.analysis.FunctionRegistry.builtin.copy(), self.executionHive)
+      // TestHiveFunctionRegistry tracks removed functions. So, we cannot simply use


Actually I do not fully understand this comment.

SparkQA · 2016-04-05T02:49:48Z

Test build #54931 has finished for PR 12117 at commit 88fd93c.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2016-04-05T03:36:21Z

Test build #54945 has finished for PR 12117 at commit 8a41f6d.

This patch fails MiMa tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2016-04-05T05:28:35Z

Test build #54948 has finished for PR 12117 at commit 361421c.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

viirya · 2016-04-05T05:36:07Z

LGTM now.

SparkQA · 2016-04-05T16:16:01Z

Test build #54987 has finished for PR 12117 at commit 3938766.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

andrewor14 · 2016-04-05T19:25:59Z

sql/core/src/main/scala/org/apache/spark/sql/execution/command/functions.scala

-        }
+      if (!ifExists && !catalog.functionExists(func)) {
+        throw new AnalysisException(
+          s"Function '$functionName' does not exist in database '$dbName'.")


I still think this should be handled within dropFunction itself (by passing the ifExists flag), but not a big deal

oh yes. I totally agree. We should do it.

https://issues.apache.org/jira/browse/SPARK-14410

andrewor14 · 2016-04-05T19:26:33Z

Thanks @yhuai @viirya I'm merging this into master.

## What changes were proposed in this pull request? This is a followup to #12117 and addresses some of the TODOs introduced there. In particular, the resolution of database is now pushed into session catalog, which knows about the current database. Further, the logic for checking whether a function exists is pushed into the external catalog. No change in functionality is expected. ## How was this patch tested? `SessionCatalogSuite`, `DDLSuite` Author: Andrew Or <andrew@databricks.com> Closes #12198 from andrewor14/function-exists.

viirya and others added 26 commits March 27, 2016 13:15

init import.

7d00184

Merge remote-tracking branch 'upstream/master' into native-ddl-function

9af70af

Fix hive temp function feature.

35ad7ae

Fix scala style.

cb29f0f

Use HiveFunctionRegistry to find Generator function in HiveSqlParser.

77848c9

Fix describe function problem.

b8dda84

Fix scala style.

ee957db

Fix test.

133ce1a

Merge remote-tracking branch 'upstream/master' into native-ddl-function

6b76980

Conflicts: sql/core/src/main/scala/org/apache/spark/sql/execution/command/ddl.scala

Address some comments.

e05b108

Add resources to CatalogFunction.

314c4db

Let Spark load Hive permanent function and create expression.

c370c47

A little refactoring.

acf9299

Refactoring.

2cab41c

Fix compilation error.

65d9dbd

Merge remote-tracking branch 'upstream/master' into native-ddl-function

05709f0

Conflicts: sql/core/src/main/scala/org/apache/spark/sql/execution/SparkQl.scala

Rename alias to className

67df04f

Make SessionCatalog handle makeFunctionBuilder and remove HiveFunctio…

b67c444

…nRegistry

functionClassName => className

1753cac

Put loadFunctionResources to SessionCatalog

a2d588f

SessionCatalog loads permanent function instead of letting function r…

51b72dd

…egistry loads permanent function.

Take care UnresolvedGenerator and cleanup.

66c5261

If we cannot find a function in Spark's FunctionRegistry. We will als…

2aa3725

…o try Hive's.

Merge remote-tracking branch 'upstream/master' into function

e343087

Fix tests and pre-load two Hive's builtin functions.

8154359

Merge remote-tracking branch 'upstream/master' into function

e0570cd

yhuai added 2 commits April 1, 2016 15:07

Cleanup

04a5926

Fix test.

979b03e

andrewor14 reviewed Apr 4, 2016
View reviewed changes

Merge remote-tracking branch 'upstream/master' into function

611fe17

viirya reviewed Apr 5, 2016
View reviewed changes

Address comments.

88fd93c

viirya reviewed Apr 5, 2016
View reviewed changes

Make resource loader a transient lazy val.

8a41f6d

Review comments

361421c

yhuai added 2 commits April 5, 2016 07:15

Merge remote-tracking branch 'upstream/master' into function

21ffafc

Remove unnecessary changes.

3938766

andrewor14 reviewed Apr 5, 2016
View reviewed changes

asfgit closed this in 72544d6 Apr 5, 2016

yhuai deleted the function branch April 5, 2016 20:52

andrewor14 mentioned this pull request Apr 6, 2016

[SPARK-14410][SQL] Push functions existence check into catalog #12198

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-14123] [SPARK-14384] [SQL] Handle CreateFunction/DropFunction #12117

[SPARK-14123] [SPARK-14384] [SQL] Handle CreateFunction/DropFunction #12117

yhuai commented Apr 1, 2016

SparkQA commented Apr 1, 2016

SparkQA commented Apr 1, 2016

andrewor14 Apr 4, 2016

yhuai Apr 5, 2016

andrewor14 commented Apr 5, 2016

viirya commented Apr 5, 2016

viirya Apr 5, 2016

viirya Apr 5, 2016

yhuai Apr 5, 2016

SparkQA commented Apr 5, 2016

SparkQA commented Apr 5, 2016

SparkQA commented Apr 5, 2016

viirya commented Apr 5, 2016

SparkQA commented Apr 5, 2016

andrewor14 Apr 5, 2016

yhuai Apr 5, 2016

yhuai Apr 5, 2016

andrewor14 commented Apr 5, 2016

[SPARK-14123] [SPARK-14384] [SQL] Handle CreateFunction/DropFunction #12117

[SPARK-14123] [SPARK-14384] [SQL] Handle CreateFunction/DropFunction #12117

Conversation

yhuai commented Apr 1, 2016

What changes were proposed in this pull request?

How was this patch tested?

TODOs

SparkQA commented Apr 1, 2016

SparkQA commented Apr 1, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewor14 commented Apr 5, 2016

viirya commented Apr 5, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Apr 5, 2016

SparkQA commented Apr 5, 2016

SparkQA commented Apr 5, 2016

viirya commented Apr 5, 2016

SparkQA commented Apr 5, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewor14 commented Apr 5, 2016